receiver: avoid race of hashring #1371
Merged
+34
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@squat PTAL |
squat
reviewed
Aug 5, 2019
pkg/receive/handler.go
Outdated
| @@ -140,6 +143,9 @@ func (h *Handler) StorageReady() { | |||
| // Hashring sets the hashring for the handler and marks the hashring as ready. | |||
| // If the hashring is nil, then the hashring is marked as not ready. | |||
| func (h *Handler) Hashring(hashring Hashring) { | |||
| h.mtx.Lock() | |||
| defer h.mtx.Unlock() | |||
|
|
|||
| if hashring == nil { | |||
| atomic.StoreUint32(&h.hashringReady, 0) | |||
There was a problem hiding this comment.
Now that we have introduced a new syncronization mechanism, we can get rid of the hashringReady field, which we were accessing atomically to determine hashring readiness. Instead, isReady can do a RLock, e.g.
h.mtx.RLock()
hr := h.Hashring != nil
h.mtx.RUnlock()
return sr > 0 && hr
squat
reviewed
Aug 5, 2019
pkg/receive/handler.go
Outdated
| // It is possible that hashring is ready in testReady() but become unready now, | ||
| // so we need to lock here. | ||
| if h.hashring == nil { | ||
| h.mtx.RLock() |
squat
reviewed
Aug 5, 2019
| @@ -400,14 +417,25 @@ func (h *Handler) replicate(ctx context.Context, tenant string, wreq *prompb.Wri | |||
| wreqs := make(map[string]*prompb.WriteRequest) | |||
| replicas := make(map[string]replica) | |||
| var i uint64 | |||
|
|
|||
| h.mtx.RLock() | |||
There was a problem hiding this comment.
Just for clarity, plz move this locking below the comment.
squat
reviewed
Aug 5, 2019
| @@ -275,6 +281,15 @@ func (h *Handler) receive(w http.ResponseWriter, r *http.Request) { | |||
| func (h *Handler) forward(ctx context.Context, tenant string, r replica, wreq *prompb.WriteRequest) error { | |||
| wreqs := make(map[string]*prompb.WriteRequest) | |||
| replicas := make(map[string]replica) | |||
|
|
|||
| h.mtx.RLock() | |||
There was a problem hiding this comment.
Let's move this below the comment.
Signed-off-by: YaoZengzeng <[email protected]>
|
@squat Updated. |
squat
approved these changes
Aug 5, 2019
|
cc @brancz for approval and merge |
brancz
approved these changes
Aug 5, 2019
|
Thanks a lot for finding and fixing this @YaoZengzeng! |
paulfantom
added a commit
to paulfantom/thanos
that referenced
this pull request
Aug 7, 2019
* master: iter.go: error message typo correction (thanos-io#1376) Fix usage of $GOPATH in Makefile (thanos-io#1379) Moved Prometheus 2.11.1 and TSDB to 0.9.1 (thanos-io#1380) Store latest config hash and timestamp as metrics (thanos-io#1378) pkg/receive/handler.go: log errors (thanos-io#1372) receive: Hash-ring metrics (thanos-io#1363) receiver: avoid race of hashring (thanos-io#1371) feat compact: added readiness Prober (thanos-io#1297) Add changelog entry for S3 option (thanos-io#1361) Multipart features (thanos-io#1358) Added katacoda.yaml (thanos-io#1359) Remove deprecated option from example (thanos-io#1351) Move suggestion about admin API to appropriate place (thanos-io#1355)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: YaoZengzeng [email protected]